Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use const generics for the PackedCompare control byte #39

Merged
merged 1 commit into from
May 6, 2021

Conversation

lqd
Copy link
Contributor

@lqd lqd commented Apr 17, 2021

Hi Jake 👋.

I was looking at this crate in the stdarch crater results, and I'd thought I'd try to fix it since it's yours :) I'm not sure if it's still super active, or if the proposed solution would be acceptable to you (as it uses const generics and thus is technically a breaking change by bumping the MSRV). There may be other acceptable solutions of course, but this seemed clean enough to ask for your opinion.

Some generic explanations follow:


Your crate came up in the results of a crater run updating a part of libcore: stdarch, and this PR should fix that.

For context, we're trying to update libcore's stdarch to improve handling of immediate mode arguments (tracking issue: rust-lang/stdarch#248): the current solution is suboptimal. It uses macros to ensure the immediate arguments are indeed constants, and that causes problems (the size of libcore and the time it takes to compile).

Now that const generics are available, we're trying to move from the previous way of doing things to using const generics, so that non-constant immediates cause a compile error instead:

  • rustc was updated to add a #[rustc_legacy_const_generics] mechanism to keep the unexpectedly stable intrinsics using the old method working (but min_const_generics are not exactly equivalent: associated consts require the full const_generics). So this would be a breaking change for the few crates where this const generics difference is visible, like jetscii.
  • the intrinsics in stdarch were updated (tracking issue for this effort: Remove all uses of #[rustc_args_required_const] rust-lang/stdarch#1022)
  • this revision of stdarch is currently being tested in crater, via the Bump stdarch submodule rust-lang/rust#83278 PR.

(There is also more discussion around this topic in rust-lang/rust#83167 if that interests you, but it is more about the best way to keep existing code working, whether a switch to const generics should be done over an edition, and so on.)

Overall, jetscii would stop compiling if that PR above were to be merged: the PackedCompare::cmpestrm{i} methods are generic and use the _mm_cmpestrm{i} intrinsics. However, they pass the control byte immediate via a generic constant and that doesn't work with min_const_generics and #[rustc_legacy_const_generics].

So, this PR adds a const generic parameter to the PackedCompare struct, and removes the CONTROL_BYTE associated constant from PackedCompareControl. It's less self-contained than before, that's for sure...

I've ran the tests in this crate using the rustc CI artifacts from that PR: thanks to rustup-toolchain-install-master, with the 9cae4dca0b4ddbfe82616f4d5f1090fc817e2ffa rustc CI revision.

This allows using the const generics version of stdarch
@Amanieu
Copy link

Amanieu commented May 5, 2021

@shepmaster Ping! Any update on this?

@shepmaster shepmaster merged commit c747752 into shepmaster:master May 6, 2021
@shepmaster
Copy link
Owner

Thanks!

@lqd lqd deleted the const_stdarch branch May 6, 2021 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants